New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Datagram size, not packet size #4214
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this. This is fixing something that we really should have done ages ago.
draft-ietf-quic-transport.md
Outdated
|
||
QUIC provides an acknowledged PL, therefore a sender does not implement the | ||
QUIC provides an acknowledged PL, therefore a sender does not implement a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consistency: above you say "QUIC is an acknowledged PL"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions, but I'm not finding anything incorrect.
draft-ietf-quic-transport.md
Outdated
A UDP datagram can include one or more QUIC packets. The datagram size refers to | ||
the total UDP payload size of a single UDP datagram carrying QUIC packets. That | ||
is, the datagram size includes includes the QUIC headers and protected payload, | ||
but not the UDP or IP headers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the name has changed, I don't think the extra explanation is necessary.
A UDP datagram can include one or more QUIC packets. The datagram size refers to | |
the total UDP payload size of a single UDP datagram carrying QUIC packets. That | |
is, the datagram size includes includes the QUIC headers and protected payload, | |
but not the UDP or IP headers. | |
A UDP datagram can include one or more QUIC packets. The datagram size refers to | |
the total UDP payload size of a UDP datagram carrying one or more QUIC packets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the extra detail here, it's worth spelling it out precisely since we use the concept a fair bit below. I'll take the "one or more QUIC packets" though.
draft-ietf-quic-transport.md
Outdated
fields. The PMTU can depend on path characteristics, and can therefore change | ||
over time. The largest UDP payload an endpoint sends at any given time is | ||
referred to as the endpoint's maximum packet size. | ||
includes QUIC packet headers, protected payload, and any authentication fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear if this is written with a single QUIC packet in mind or multiple. Suggestion follows.
includes QUIC packet headers, protected payload, and any authentication fields. | |
includes one or more QUIC packets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed "header" to "headers" to reflect that, but I can change it to "one or more QUIC packet headers, ...". I think it's worth spelling out the fields though.
Co-authored-by: Martin Thomson <mt@lowentropy.net>
draft-ietf-quic-transport.md
Outdated
is, the datagram size includes includes the QUIC headers and protected payload, | ||
but not the UDP or IP headers. | ||
the total UDP payload size of a single UDP datagram carrying QUIC packets. The | ||
datagram size includes includes one or more QUIC packet headers and the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
datagram size includes includes one or more QUIC packet headers and the | |
datagram size includes includes one or more QUIC packet headers and |
draft-ietf-quic-transport.md
Outdated
includes one or more QUIC packet headers, the protected payloads, and any | ||
authentication fields. The PMTU can depend on path characteristics, and can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can drop "authentication fields" as that is just part of "protected payload".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the one erronous exchange of packet this looks good.
draft-ietf-quic-transport.md
Outdated
networks. Assuming the minimum IP header size, this results in a QUIC maximum | ||
packet size of 1232 bytes for IPv6 and 1252 bytes for IPv4. | ||
networks. Assuming the minimum IP header size, this results in a maximum | ||
datagram size of 1232 bytes for IPv6 and 1252 bytes for IPv4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it should say "IP packet" instead of datagram as it is the IP packet sizes for IPv4 and IPv6 that are given.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reading, I think this might have been better as QUIC maximum packet size, since that is accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't the IP packet size or the QUIC packet size -- this really is about the UDP payload size (1280 - 20/40 - 8 = 1252/1232). I'm leaving this as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now spelling this out a bit more, so the computation is crystal clear.
draft-ietf-quic-transport.md
Outdated
networks. Assuming the minimum IP header size, this results in a QUIC maximum | ||
packet size of 1232 bytes for IPv6 and 1252 bytes for IPv4. | ||
networks. Assuming the minimum IP header size, this results in a maximum | ||
datagram size of 1232 bytes for IPv6 and 1252 bytes for IPv4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reading, I think this might have been better as QUIC maximum packet size, since that is accurate.
Addresses #4196.
@gloinul points out in #4196 that we use
maximum packet size
in the transport draft andmax_datagram_size
in the recovery draft. In reading carefully, I realized thatmaximum packet size
in the transport draft was actually historical, andmaximum datagram size
is the more appropriate term.This is an entirely editorial PR, and it's largely simple changes, despite the fact that it looks a bit big. It is a bit subtle though. Sorry about that.